cuda.bindings: support multiple CTK release lines on main - #2737
Conversation
Moon migration belongs in PR NVIDIA#2659. Restore the pre-Moon selective-CI planner and workflows from 727ef59.
|
/ok to test 22108c1 |
Treat carriage returns as delimiters in both jq TSV reads. Native jq on Windows emits CRLF, which otherwise leaves the final field contaminated and silently disables maintenance-line cuda.core Cython test artifacts.
Synchronize the applicable thread-safety markers from NVIDIA#2229 into the maintenance test tree. NVML initialization and graph-memory accounting use process-global state and must not run alongside parallel tests under free-threaded Python.
|
/ok to test 889062e |
|
/ok to test 7e2b151 |
…po are identical This avoids: * hard-coded LICENSE filepaths. * checking the same LICENSE files multiple times, depending on how pre-commit batches the files.
|
To log this here: To make manual review more tractable, I created this 6-layer review-only branch: https://github.com/rwgk/cuda-python/commits/review/multi-ctk-bindings-on-main-20260902-2224-7e2b151 There are six narrated layers:
Verified:
|
|
/ok to test 09f27fb |
|
/ok to test 7cb81e1 |
rwgk
left a comment
There was a problem hiding this comment.
Hi @mdboom, I rebuilt the six-layer narrated review branch a third time from the current PR head (7cb81e1):
https://github.com/rwgk/cuda-python/commits/review/multi-ctk-bindings-on-main-20260903-1844-7cb81e1
That branch is only a review aid; this PR remains authoritative. The six commits are meant to be read oldest to newest. Each commit message explains its boundary, suggested review focus, and validation:
- 6abe8bb — import the maintained CUDA 12.9 source root
- dbbdf68 — define the package-root/release-status registry
- 8be614a — plan selective work by package root
- 61e67bb — route builds and tests
- c1bad81 — resolve and validate releases
- 04bfed6 — retire the 12.9 backport workflow
For the shortest useful path, I'd start with layer 2 and then review layers 3-5. Layer 4 is necessarily the broad workflow-integration layer.
I incorporated your latest feedback: ci.tools is now a small package under ci/pyproject.toml; downstream consumers trust normalized package records; pre-commit checks the registry against both bindings packages' metadata; and the generic --package-json behavior was replaced by the purpose-specific write-github-env command.
I then made another simplification pass, removing unsupported planner shapes, unused workflow fallbacks, repeated registry reads, and redundant release metadata/checks while preserving historical-tag compatibility. That pass changed 280 lines in and 412 lines out - a net reduction of 132 lines, including the added focused tests.
The current PR head is fully green. When you have a chance, could you please review this fresh layered view and let me know whether you see anything else blocking this PR? The main decisions I hope to settle are the package-root/release-status model, release routing, and moving CUDA 12.9 maintenance onto main.
| moves to a new toolkit minor; registry validation rejects a configuration | ||
| where the two disagree. | ||
|
|
||
| The Python helpers share registry parsing and validation, so they are modules |
There was a problem hiding this comment.
This implies that (a) you are running from the root of the repo, and (b) you already did a
pip install -r ci/tools/requirements.txt.
requirements.txthas been deprecated for a long time. We should either: add apyproject.tomlforci/toolsand require that the package is installed into a venv (which removes the$PWDproblem), or use PEP 723 comments in each script and not have a proper package altogether.Personally, I think PEP 723 is the simpler solution (it doesn't require juggling virtual environments), but it does require having
uvorpixiaround to run the script.The argument in favor of a package would be if these scripts start to share a lot of logic and helpers between them such that things in ci/tools need to import other things in ci/tools. It doesn't look like they do right now, but in the original mega-PR they started to do that a bit.
Agreed. I chose the package option after the later refactor made the helpers genuinely share logic: bindings_scm_version, check_pixi_cuda_version, check_release_notes, and compute_ci_plan now import the registry/version machinery rather than duplicating it. The PR now adds ci/pyproject.toml, removes both requirements files, and installs ./ci in editable mode at workflow call sites. The documented module invocation therefore works through an installed package and no longer relies on $PWD putting the repository on Python's import path; PEP 723 would repeat the same dependency metadata across several scripts.
| parser.add_argument("--control-config", type=Path) | ||
| commands = parser.add_subparsers(dest="command") | ||
| write_env = commands.add_parser( | ||
| "write-github-env", |
There was a problem hiding this comment.
What's the purpose of this flag? It looks like it just spits the JSON provided right back out, doing nothing to it?
Agreed--it had no useful standalone meaning. --package-json and --github-env are gone. The only JSON-consuming operation is now the purpose-specific write-github-env GITHUB_ENV subcommand, which reads a selected package record from stdin, validates it, and appends only the supported environment variables. This removes the round-trip behavior and prevents that operation from being accidentally combined with registry-selection modes.
| types: [text] | ||
|
|
||
| - id: check-bindings-config | ||
| name: Check bindings registry tracks package metadata |
There was a problem hiding this comment.
This duplication from
versions.ymlis worrisome, but I don't see any way around it. I think a precommit hook to make sure it's in sync withversions.ymlwould be a good idea.
Agreed. I added this check-bindings-config hook, triggered by ci/versions.yml or any bindings pyproject.toml. Registry validation reads each package root's tag_regex, constructs v<toolkit_version> from the registry, and requires the SCM regex to accept that exact version. Focused SCM tests additionally verify that the current 13.3 pattern rejects 13.4 and that metapackage SCM metadata matches both bindings roots. A normal release-family update on either side now fails locally and in CI if the duplicated metadata drifts.
kkraus14
left a comment
There was a problem hiding this comment.
Codex review at 7cb81e114f8f4fabe3e4fa35a9f2bb5c11b5a45a. I left inline comments for the code findings and temporary-ignore cleanup. Two branch-wide blockers do not have a meaningful single-line anchor: this head currently conflicts with main in the backport/workflow files, so it needs to be updated and CI rerun; and I could not find a CI: Release 12.9 dry run for this head, so the maintenance release path is not yet validated end to end. The successful normal CI run rebuilt both bindings lines because CI files changed, so it does not cover the mixed selective-build case called out inline. The pipeline-diagram follow-up is already covered by the existing review thread and #2740.
| - name: Download cuda.bindings build artifacts | ||
| if: ${{ (env.TEST_BINDINGS == 'true' || env.TEST_CORE == 'true' || env.TEST_PYTHON == 'true') && | ||
| env.BINDINGS_SOURCE == 'main' }} | ||
| env.BINDINGS_SOURCE == 'local' }} |
There was a problem hiding this comment.
Codex review — High: this gate consumes the aggregate TEST_BINDINGS / TEST_CORE / TEST_PYTHON flags for every CUDA-major matrix row. For a mixed change such as a v12 bindings file plus a cuda_core test, the plan builds only the v12 bindings test artifact but schedules core tests for both v12 and v13; the v13 row then reaches this step and requests a v13 artifact that was never built. The Windows workflow has the same shape. Please derive these gates per selected bindings line (or otherwise ensure each local row only downloads an artifact its root built) and add a mixed-change regression test.
| ) | ||
|
|
||
| missing_distributions = sorted(expected_distributions - set(seen_versions)) | ||
| missing_distributions = sorted(expected_distributions - seen_distributions) |
There was a problem hiding this comment.
Codex review — High: this verifies only that each distribution name appeared. I reproduced a successful v12 validation with a directory containing just one correctly named wheel, even though a release should contain every supported Python/platform/ABI variant. That can allow a partial TestPyPI/PyPI release after an artifact is missing, deleted, or expired. Please validate the complete expected wheel matrix (or exact expected artifact set from the build plan) before upload and add a partial-matrix rejection test.
| else | ||
| echo "error: cannot find ci/versions.yml or ci/versions.json" >&2 | ||
| exit 1 | ||
| resolved_bindings_package=$(cd .ci-control && python3 -m ci.tools.bindings_config \ |
There was a problem hiding this comment.
Codex review — Medium: cuda-core-v* releases reach this fallback because release.yml does not pass bindings-package for cuda-core and the branch above recognizes only bare v* tags. This selects current control-plane bindings metadata instead of the metadata in the release tag. A delayed or historical core release can therefore request a newer bindings artifact that its tagged CI run never built (for example, cuda-core v1.0.0 used CUDA 13.2.1 while current control uses 13.3.0). Please resolve the bindings dependency from the checked-out release tag registry for component-prefixed releases too.
| if not release_source_root.is_dir(): | ||
| raise BindingsConfigError(f"release source root is not a directory: {release_source_root}") | ||
|
|
||
| tagged_config_path = release_source_root / "ci" / "versions.yml" |
There was a problem hiding this comment.
Codex review — Medium: real v12.9.1–v12.9.7 and v13.0.x tags store this metadata in ci/versions.json, not ci/versions.yml. Treating the YAML as absent sends those tags to the legacy fallback, which derives the line from today's control config; that only works while today's pins happen to match. Please probe and parse both legacy filenames from the tag tree before using control metadata, with a fixture shaped like an actual old tag.
| tag_regex = "^(?P<version>v\\d+\\.\\d+\\.\\d+(?:[ab]\\d+)?)" | ||
| git_describe_command = ["git", "describe", "--dirty", "--tags", "--long", "--match", "v*[0-9]*"] | ||
| # Accept the project's prerelease, post-release, and development suffixes. | ||
| tag_regex = "^(?P<version>v13\\.3\\.\\d+(?:(?:a|b|rc)\\d+)?(?:\\.post\\d+)?(?:\\.dev\\d+)?)$" |
There was a problem hiding this comment.
Codex review — Medium: accepting .devN here makes a development tag valid for tag CI, but cuda_bindings/docs/build_docs.sh truncates the version to the first three dot components. As a result, v13.3.0.dev1 deploys production docs under 13.3.0, potentially occupying or overwriting the stable path even though release-wheel validation rejects development releases. Please retain the dev suffix in the docs path or prevent production docs deployment for dev tags.
| ## Installing from PyPI | ||
|
|
||
| ```console | ||
| $ pip install -U cuda-python |
There was a problem hiding this comment.
Codex review — Medium: this is the CUDA 12 maintenance documentation, but the unpinned metapackage command now resolves the current v13 line. The [all] and Conda examples have the same issue, and the copied README/DESCRIPTION links target /cuda-bindings/latest/ (v13). Please pin the maintenance line (for example, cuda-python~=12.9.0 or cuda-bindings~=12.9.0) and direct readers to the versioned 12.9 docs or version selector.
| cuda_devices = get_cuda_device_names() | ||
| nvml_devices = get_nvml_device_names() | ||
|
|
||
| assert cuda_devices == nvml_devices, "CUDA and NVML device lists do not match" |
There was a problem hiding this comment.
Codex review — Medium: this copied v12 test omits the fixes already present in the current-root test for Orin/Thor and CUDA_VISIBLE_DEVICES. On a multi-GPU host with CUDA_VISIBLE_DEVICES=0, CUDA reports one visible GPU while NVML reports all GPUs, so this exact equality fails. Please port the current-root handling to the v12 copy, consistent with the cross-root handwritten-fix policy in cuda_bindings_12/MAINTENANCE.md.
| git(tmp_path, "init") | ||
| git(tmp_path, "config", "user.name", "CUDA Python CI") | ||
| git(tmp_path, "config", "user.email", "cuda-python@nvidia.com") | ||
| git(tmp_path, "config", "commit.gpgsign", "false") |
There was a problem hiding this comment.
Codex review — Low: this temporary repository disables commit signing but still inherits global tag signing. With tag.gpgSign=true, the plain git tag calls fail or prompt (I reproduced 14 failures); with tag signing disabled, the same suite passes. Please set tag.gpgSign=false here and in the analogous temp-repo helper in test_lookup_run_id.py, or create tags with --no-sign, so the tests are hermetic.
| @@ -0,0 +1,7 @@ | |||
| # REMOVE THIS FILE BEFORE MERGING THIS PR | |||
There was a problem hiding this comment.
Codex review — Pre-merge cleanup: this temporary file is still tracked and suppresses link checks for paths that do not yet exist on main. Please remove it before the final CI run and merge, as its header and the PR checklist require.
REMINDER
Before merging, remove the temporary
.lycheeignorebefore triggering final CI. It excludes only three canonicalmain/cuda_bindings_12URLs that cannot resolve until this PR is merged. The authored-sourcelycheepre-commit hook is explicitly skipped by the CI pre-commit jobs; CI separately checks links in rendered documentation. Removing this file therefore will not introduce a required-check failure before merge.After merging, run
pre-commit run lychee --all-fileson freshmainto validate those links.Summary
Closes #1199.
This is the writable continuation of Keith Kraus's original PR #2675, "cuda.bindings: build 12.9 and 13.x selectively from main". Keith authored most of the CUDA 12 source import and the initial build, test, and release integration. GitHub closed #2675 automatically when its temporary base branch was deleted after #2467 merged; this replacement preserves that work and incorporates the redesigns requested so far.
This PR moves active development of both released CUDA bindings lines onto
main:cuda_bindings_12/maintenancecuda_bindings/currentThe two roots are alternative source trees for the same
cuda-bindingsdistribution andcuda.bindingsimport namespace. Either can be built and installed independently, but they are not separately named or intended to coexist in one environment. CI, documentation, and release workflows select the appropriate source root and toolkit pin. After merge, the historical12.9.xbranch remains only as a read-only release record, not an active backport or artifact-source branch.Narrated Review Branch
For human review, this six-commit branch is tree-identical to review snapshot 7cb81e1:
review/multi-ctk-bindings-on-main-20260903-1844-7cb81e1
The PR branch remains authoritative. Read the review branch oldest to newest; each commit message explains its boundary, suggested review focus, and final-tree validation:
The highest-value review path is layers 2-5. Layer 1 is predominantly Keith's imported CUDA 12.9 tree; layer 6 isolates the branch-ownership policy. The intermediate commits are a narrative decomposition, not independently deployable states.
Design
ci/versions.ymlmaps each unique root directly to its exact toolkit pin and scalarcurrentormaintenancestatus. There are no synthetic line IDs or separate role-to-line mappings.[tool.setuptools_scm].tag_regexdefines its accepted release family. Registry validation reads that metadata and checks that it agrees with the configured toolkit pin; it derives the CTK target and CUDA ABI fields.currentroot and onemaintenanceroot with different CUDA ABI majors. Other shapes fail rather than implying workflow support that does not exist.ci.toolspackage centralizes registry, version, planner, and release logic. Build, test, sdist, source-smoke, coverage, nightly, documentation, and release workflows consume explicit normalized inputs instead of reconstructing bindings identity from directory names or hard-coded CUDA majors.main. A contained compatibility path handles historical tags whose trees predate the registry; malformed modern registries and unknown tags fail closed. Artifact selection excludes test-only wheels, and release validation rejects missing, mismatched, development/local, or unexpected distributions.cuda_bindings_12/MAINTENANCE.mdrecords the imported tree's source and partial cybind-generation provenance. The roots are not required to be byte-identical: generated and target-specific differences may be legitimate, while handwritten changes require an explicit applicability review across both roots.Review Feedback Incorporated
ci/pyproject.toml; downstream consumers trust normalized records, and build setup reads the registry once.Reviewer Decisions
Please explicitly accept or reject these policies:
mainis the sole active source of truth. The historical12.9.xbranch receives no further routine or emergency backports. CUDA 12 fixes are made incuda_bindings_12/onmain, alongside a corresponding current-root change when applicable.release_statusrecords its place in the release lifecycle without introducing a second line identity.Validation
The following validation completed successfully for review snapshot 7cb81e1:
pre-commit run --all-files: passed, including Ruff, actionlint, YAML/TOML/RST checks, generated-file seals, SCM/registry checks, and authored-sourcelycheewith the three temporary exact exclusions described in the REMINDERView all PR checks, the completed main CI run, and the selected-source smoke run.
Out of Scope
Checklist
ci/.ci-pipeline-regen.mdand regenerateci/ci-pipeline.svgto reflect the current pipeline, including removal of the retired backport artifact flows..lycheeignorebefore final CI and merging, as described in the REMINDER.